Skip to content

flask-{wtf,admin} fixes#156188

Merged
SuperSandro2000 merged 3 commits intoNixOS:masterfrom
arcnmx:flask-wtf-fixes
Feb 2, 2022
Merged

flask-{wtf,admin} fixes#156188
SuperSandro2000 merged 3 commits intoNixOS:masterfrom
arcnmx:flask-wtf-fixes

Conversation

@arcnmx
Copy link
Member

@arcnmx arcnmx commented Jan 22, 2022

Motivation for this change

#155221 (comment) broke a number of packages, most of which haven't been updated or released with support for wtforms 3.0 yet. This selectively overrides the dependency for the affected packages.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

cc @gador @SuperSandro2000

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 22, 2022
@arcnmx arcnmx mentioned this pull request Jan 22, 2022
13 tasks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meta should be last.

Comment on lines 19 to 23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
patches = lib.singleton (fetchpatch {
name = "fix-wtforms3.patch";
url = "https://github.com/coleifer/wtf-peewee/commit/b1764f4474c73a9a2b34ae6b7db61274f5252a7f.patch";
sha256 = "0maz3fm9bi8p80nk9sdb34xq55xq8ihm51y7k0m8ck9aaypvwbig";
});
patches = [
(fetchpatch {
name = "fix-wtforms3.patch";
url = "https://github.com/coleifer/wtf-peewee/commit/b1764f4474c73a9a2b34ae6b7db61274f5252a7f.patch";
sha256 = "0maz3fm9bi8p80nk9sdb34xq55xq8ihm51y7k0m8ck9aaypvwbig";
})
];

Comment on lines 38 to 47
Copy link
Member

@SuperSandro2000 SuperSandro2000 Jan 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how we do such things in pythonPackages. I assume the packages share the module name which means we can't do it like this and need to overwrite it for every end application that we want to fix. This is due to limitations in python which we did not come around yet.

Also if we would add another version it would be in top-level named wtforms_2.

Copy link
Member Author

@arcnmx arcnmx Jan 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to suggest an alternative, but I'd rather revert #155221 and override it for the single package that needs 3.0 (which hasn't even been merged yet?) rather than copy-paste this into like 10 different packages :(

I agree that this isn't great, but it's meant as a stop-gap to be removed once the rest of the ecosystem has been updated. Copying it all would hurt me too much to submit, though feel free to amend this PR accordingly (I think that's how that github checkbox works?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is is forbidden. We cannot use it as a stop gap. It causes massive issues when the two versions end up in a single python env. We can also revert the update of wtforms and override it for the one package that actually needs it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check if we can fetch spiral-project/ihatemoney#916.

Comment on lines 23 to 25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
passthru.wtforms2 = flask_wtf.override {
wtforms = wtforms.wtforms2;
};

I don't think we even need this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well unless you're applying an overlay to the python package set, anything that indirectly depends on wtforms must also be overridden to keep it out of the closure.

But yes, this can instead be overridden manually everywhere flask_wtf is used, if that's what you mean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jan 22, 2022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check if dpgaspar/Flask-AppBuilder#1734 is already enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

airflow is so often broken, I don't really care anymore at this point. I would suggest to just leave it like it is and let someone else care about it.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 22, 2022
@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 22, 2022
@arcnmx
Copy link
Member Author

arcnmx commented Jan 22, 2022

@SuperSandro2000 pulled in patches for most of it instead, and omitted airflow

@symphorien
Copy link
Member

I'm taking care of ihatemoney in #156350 (it needs some more things because of sqlalchemy-continuum)

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 23, 2022
@ofborg ofborg bot removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jan 23, 2022
Comment on lines 27 to 34
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't do this. It will cause issues which will be hard to debug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so... leave it broken or what do you want here?
buku appears to be the only user of this module, so the override could stay isolated to that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes for python modules (as opposed to leaf packages) it's not possible to override their dependencies. So by bumping wtforms to 3, flask-admin was broken without solution. The only solution to prevent that is not to merge breaking updates in the first place. Please consider that next time before merging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help, if we add the version bump of WTForms as an extra package wtforms3? So everything that still relies on WTForms2 could use the original package, any any package needing the version 3, could use wtforms3?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help, if we add the version bump of WTForms as an extra package wtforms3? So everything that still relies on WTForms2 could use the original package, any any package needing the version 3, could use wtforms3?

If they both end up in the same pythonEnv things still break because they share the same module name internally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what an "empty" package means.

A package that contains no files. That happens when pip installs a package to $out which is already in the environment and then nothing happens except that $out is created and some directories and that ends up being the package.

of course once flask-admin is properly updated then this override would be obviously unneeded and removed.

Lets be real, there is a high chance someone will forget.

I'm not really aware of python version overrides being uncommon practice in nixpkgs

We nuked most of them from the tree a few months ago so except pytest there should be almost none.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what an "empty" package means.

A package that contains no files. That happens when pip installs a package to $out which is already in the environment and then nothing happens except that $out is created and some directories and that ends up being the package.

Well that's unfortunate as a failure case, but is a much more extraordinary case that isn't relevant here - plus could easily be guarded against if it were a real concern. A package that accidentally depends on itself if it weren't overridden would cause infinite recursion on evaluation, no? A situation where that is narrowly avoided due to a stray override seems like it would be an incredibly exceptional one. You'd also hope installtests would catch it, but .-.

of course once flask-admin is properly updated then this override would be obviously unneeded and removed.

Lets be real, there is a high chance someone will forget.

Sorry, I want to say I highly doubt your statement here, as it would be immediately obvious to anyone looking at the file to update it. Perhaps not to newcomers, though? and then if github doesn't show it in the diff... though, you could apply that as an argument to reject literally any change in nixpkgs, so it doesn't feel very useful?

I'm not really aware of python version overrides being uncommon practice in nixpkgs

We nuked most of them from the tree a few months ago so except pytest there should be almost none.

Gotcha, well it sucks that this was apparently done without any alternatives proposed. Is any of this policy documented somewhere?
(but really, the fact that nixpkgs allows merges to break any package is honestly the real problem here, unfortunately)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a much more extraordinary case that isn't relevant here

I already got this once into master and unstable for 3 weeks and no one noticed that the checkInput poluted the environment and the package ended up being empty. 🙃

A package that accidentally depends on itself if it weren't overridden would cause infinite recursion on evaluation, no?

For nix they are two different packages but for python not and we currently do not have any safeguard for this.

Gotcha, well it sucks that this was apparently done without any alternatives proposed

Limitation of python which cannot easily and universally overcome.

Is any of this policy documented somewhere?

IDK, probably not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flask-admin got updated to version 1.6.0 which includes an update to WTForms 3:

1.6.0
Dropped Python 2 support
WTForms 3.0 support
Various fixes

I build it locally and it works flawlessly with WTForms3. The only thing I needed to change was to add flask_admin/tests/sqla/test_inlineform.py to disabledTestPaths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it seems good here, replaced the override with the update!

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 156188 run on x86_64-linux 1

8 packages built:
  • apache-airflow (python39Packages.apache-airflow)
  • python310Packages.apache-airflow
  • python310Packages.flask-admin
  • python310Packages.flask-appbuilder
  • python310Packages.wtf-peewee
  • python39Packages.flask-admin
  • python39Packages.flask-appbuilder
  • python39Packages.wtf-peewee

@SuperSandro2000 SuperSandro2000 merged commit 4c62db9 into NixOS:master Feb 2, 2022
@arcnmx arcnmx deleted the flask-wtf-fixes branch November 26, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants